fix(frontend): align memories page wrapper with route#1237
fix(frontend): align memories page wrapper with route#1237Ashvin-KS wants to merge 1 commit intoAOSSIE-Org:mainfrom
Conversation
WalkthroughThe Memories page wrapper component is updated to render the real Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Aligns the Memories route and page entrypoint so routing and page-level tests exercise the real Memories UI (fixing the placeholder wrapper issue described in #1236).
Changes:
- Update
pages/Memories/Memories.tsxto render the realMemoriesPagecomponent. - Update
AppRoutes.tsxto route to the page module (pages/Memories/Memories) instead of importingMemoriesPagedirectly from components.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| frontend/src/routes/AppRoutes.tsx | Routes ROUTES.MEMORIES to the page entrypoint (<Memories />) and updates the import accordingly. |
| frontend/src/pages/Memories/Memories.tsx | Replaces the empty placeholder render with MemoriesPage, ensuring the page wrapper exercises the actual UI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/routes/AppRoutes.tsx`:
- Line 26: Add an integration test in allPages.test.tsx (or a new test file)
that renders AppRoutes and navigates to ROUTES.MEMORIES instead of mounting
Memories directly: use a router helper (e.g., MemoryRouter or
createMemoryHistory + Router) and initialize it with the path ROUTES.MEMORIES,
render <AppRoutes /> and then assert real Memories UI behavior (for example
check for the Memories page heading, a specific button, or that the memory list
items appear) rather than just a smoke test; ensure the test imports AppRoutes,
ROUTES, and the same selectors/text used by the Memories component to validate
real routing and UI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04089976-86e1-4f13-a3ee-bb145d9826d4
📒 Files selected for processing (2)
frontend/src/pages/Memories/Memories.tsxfrontend/src/routes/AppRoutes.tsx
| <Route path={ROUTES.AI} element={<AITagging />} /> | ||
| <Route path={ROUTES.ALBUMS} element={<ComingSoon />} /> | ||
| <Route path={ROUTES.MEMORIES} element={<MemoriesPage />} /> | ||
| <Route path={ROUTES.MEMORIES} element={<Memories />} /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Scanning frontend tests for Memories route coverage via AppRoutes..."
fd -t f '(test|spec)\.(ts|tsx|js|jsx)$' frontend \
| xargs -r rg -n -C3 'AppRoutes|ROUTES\.MEMORIES|/memories|MemoriesPage|@/pages/Memories/Memories|@/components/Memories'Repository: AOSSIE-Org/PictoPy
Length of output: 386
🏁 Script executed:
fd -t f '(test|spec)\.(ts|tsx|js|jsx)$' frontend \
| xargs -r rg -l 'Page rendering tests' \
| head -5Repository: AOSSIE-Org/PictoPy
Length of output: 108
🏁 Script executed:
# Once we find the file, let's read it to see the full test implementation
fd -t f '(test|spec)\.(ts|tsx|js|jsx)$' frontend \
| xargs -r rg -l 'Page rendering tests' \
| xargs head -100Repository: AOSSIE-Org/PictoPy
Length of output: 1653
Add integration test for the Memories route through AppRoutes.
The existing allPages.test.tsx test only renders the Memories component directly with a smoke-test assertion ("renders without crashing"). This does not exercise the route through AppRoutes or verify real Memories UI behavior. Add an integration test that navigates to ROUTES.MEMORIES via AppRoutes and asserts expected Memories functionality.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/routes/AppRoutes.tsx` at line 26, Add an integration test in
allPages.test.tsx (or a new test file) that renders AppRoutes and navigates to
ROUTES.MEMORIES instead of mounting Memories directly: use a router helper
(e.g., MemoryRouter or createMemoryHistory + Router) and initialize it with the
path ROUTES.MEMORIES, render <AppRoutes /> and then assert real Memories UI
behavior (for example check for the Memories page heading, a specific button, or
that the memory list items appear) rather than just a smoke test; ensure the
test imports AppRoutes, ROUTES, and the same selectors/text used by the Memories
component to validate real routing and UI.
|
Hey thanks for contributing but currently we are closing all prs that are linked to unreviewed issues please wait for mentors to discuss the issue mention your issue with some idea in the discord channel and wait for them to assign it to you then go ahead with the pr! |
|
Thanks for the update and for clarifying the process. Understood, I’ll pause the PR for now. I’ll post the issue with my implementation idea in the Discord channel and wait for mentor review/assignment before proceeding further. Appreciate the guidance. |
Addressed Issues:
Fixes #1236
Screenshots/Recordings:
N/A (no visual behavior change intended)
Additional Notes:
This is a small frontend consistency fix:
pages/Memories/Memories.tsxnow rendersMemoriesPageAppRoutes.tsximports the Memories route from the page moduleWhy:
The page wrapper was a placeholder, which could let page-level tests pass without exercising the real Memories UI.
AI Usage Disclosure:
I have used the following AI models and tools:
Checklist
Summary by CodeRabbit